Skip to content

FIX: inactive touch driving invalid synthetic click on state reset#2349

Open
MorganHoarau wants to merge 10 commits intodevelopfrom
fix/uum-100125/inactive-touch-drive-phantom-click-on-state-reset
Open

FIX: inactive touch driving invalid synthetic click on state reset#2349
MorganHoarau wants to merge 10 commits intodevelopfrom
fix/uum-100125/inactive-touch-drive-phantom-click-on-state-reset

Conversation

@MorganHoarau
Copy link
Collaborator

@MorganHoarau MorganHoarau commented Feb 18, 2026

Description

Addresses UUM-100125 (phantom click after device rotation).

Follow up PR about a UITKInputModule failing test locally: #2367

Issue:

  • After rotation, InputSystem queue a device configuration event which causes InputActionState to run initial-state catch-up in OnBeforeInitialUpdate. (See Comments To Reviewers section for overview)
  • Touch position state can still be non-default after touch end (because position is dontReset = true). So the stale state (touch position being non-default) was treated as fresh actuation and trigger unexpected callbacks.

Changes

  • Added a guard in initial-state check to skip replay for inactive touch state.
  • Added regression test: Actions_InitialStateCheckAfterConfigurationChange_DoesNotTriggerForInactiveTouch
  • Updated existing test Actions_WithMultipleCompositeBindings_WithoutEvaluateMagnitude_Works(true) to align the new expected behaviour. The test still preserves the original ISXB-98 guarantee: after enabling, real subsequent touches must still trigger the composite action correctly.

TODO:

  • Check existing documentation around the topic (Touch, device configuration event, dontReset) - need update?

Also, unlike both Button and PassThrough actions, Value actions perform what's called "initial state check" on the first input update after the action was enabled. What this does is check controls bound to the action and if they are already actuated (that is, at non-default value), the action will immediately be started and performed. What this means in practice is that when a value action is bound to, say, the left stick on a gamepad and the stick is already moved out of its resting position, then the action will immediately trigger instead of first requiring the stick to be moved slightly.
From InputActionType documentation.

  • Changelog

Testing status & QA

  • Reproduced issue in repro project.
  • Added new regression test for this bug path.
  • Ran existing tests and identified conflict with: Actions_WithMultipleCompositeBindings_WithoutEvaluateMagnitude_Works(true)

Overall Product Risks

  • Complexity: Low
  • Halo Effect: Medium - affects TouchControl that supports initial state check.

Comments to reviewers

Tracing the whole flow can help understand the broader context. DeviceConfigurationEvent flow diagram:

flowchart TD
    subgraph Native["Native / Backend"]
        A[Device rotates]
        B[Queue DeviceConfigurationEvent]
    end

    subgraph IM["InputManager"]
        C[Process DeviceConfigurationEvent]
        D[NotifyConfigurationChanged]
        E[InputActionState.OnDeviceChange ConfigurationChanged]
    end

    subgraph IASResolve["InputActionState.Resolve phase"]
        F[Full binding re-resolve]
        G[Set initialStateCheckPending = true]
    end

    subgraph IASInitial["InputActionState.OnBeforeInitialUpdate"]
        H[Loop bindings and controls]
        I{Control belongs to TouchControl?}
        J{touch.isInProgress?}
        K[Default CheckStateIsAtDefault path]
    end

    subgraph OldPath["Old behavior"]
        L[SignalStateChangeMonitor for inactive touch]
        M[Started/Performed from stale touch state]
        N[Phantom UI click]
    end

    subgraph NewPath["Current proposed fix"]
        O[Skip inactive touch control]
        P[No synthetic Started/Performed]
        Q[No phantom UI click]
    end

    A --> B
    B --> C
    C --> D
    D --> E
    E --> F
    F --> G
    G --> H
    H --> I
    I -- no --> K
    I -- yes --> J
    J -- yes --> K
    J -- no old --> L
    L --> M
    M --> N
    J -- no new --> O
    O --> P
    P --> Q
Loading

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

Add ShouldSkipInitialStateCheck and use it in InputActionState's initial-state loop to avoid treating preserved touch data as actuated during binding re-resolution.
@MorganHoarau MorganHoarau changed the title Fixes inactive touch driving invalid synthetic click on state reset FIX: inactive touch driving invalid synthetic click on state reset Feb 18, 2026
@MorganHoarau MorganHoarau marked this pull request as ready for review March 3, 2026 09:40
@u-pr
Copy link
Contributor

u-pr bot commented Mar 3, 2026

PR Reviewer Guide 🔍

(Review updated until commit ea8b067)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

Moderate effort: small but behavior-changing runtime logic in action initial-state processing plus new/updated tests that need careful validation for determinism and intended semantics.

🏅 Score: 88

Solid, targeted fix with regression coverage, but the change alters initial-state behavior for touch bindings and one new test assertion looks potentially brittle across internal action-state variations.

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Test Robustness

The new regression test assumes exactly one trace event and that it is always Canceled after a configuration change; depending on whether the re-resolve actually results in a cancel for this action in a given runtime/path, this may be flaky (could be zero events or additional events). Consider asserting the absence of Started/Performed rather than requiring an exact count/phase, or otherwise tightening the setup to guarantee a cancel.

// Configuration change causes full re-resolve and schedules initial state check.
InputSystem.QueueConfigChangeEvent(touchscreen);
InputSystem.Update();
InputSystem.Update();

// Full re-resolve may cancel the current action state. What must NOT happen is a synthetic
// Started/Performed pair from persisted inactive touch state.
Assert.AreEqual(1, trace.count);
foreach (var eventPtr in trace)
{
    // The trace should only contain a Canceled event for the action.
    Assert.AreEqual(InputActionPhase.Canceled, eventPtr.phase, 
        $"inactive touch state should not produce action callbacks, but received {eventPtr.phase}.");
}
Behavior Change

Skipping initial-state catch-up for any control under a TouchControl when isInProgress is false prevents replay from persisted dontReset state (desired), but it also changes semantics for value bindings that may intentionally want “last known touch position/touchId” even when not in progress; confirm this is acceptable for all touch sub-controls and doesn’t break existing user expectations beyond the intended fix.

            if (ShouldSkipInitialStateCheck(control))
                continue;

            if (!control.CheckStateIsAtDefault())
            {
                // Update press times.
                if (control.IsValueConsideredPressed(control.magnitude))
                {
                    // ReSharper disable once CompareOfFloatsByEqualityOperator
                    if (bindingState.pressTime == default || bindingState.pressTime > time)
                        bindingState.pressTime = time;
                }

                // For composites, any one actuated control will lead to the composite being
                // processed as a whole so we can stop here. This also ensures that we are
                // not triggering the composite repeatedly if there are multiple actuated
                // controls bound to its parts.
                if (isComposite && didFindControlToSignal)
                    continue;

                manager.SignalStateChangeMonitor(control, this);
                didFindControlToSignal = true;
            }
        }
    }
    manager.FireStateChangeNotifications();

    k_InputInitialActionStateCheckMarker.End();
}

private static bool ShouldSkipInitialStateCheck(InputControl control)
{
    // UUM-100125
    // Touch controls intentionally preserve state such as position even when no touch is currently active.
    // During binding re-resolution this can make inactive touches look actuated and cause invalid triggers.
    for (var current = control; current != null; current = current.parent)
    {
        if (current is TouchControl touchControl)
        {
            return !touchControl.isInProgress;
        }
    }

    return false;
}
Performance/Scope

The parent-walk in ShouldSkipInitialStateCheck runs for every bound control during initial-state checks; it’s likely fine given the frequency, but validate it doesn’t become a hot spot with many bindings/controls, and consider a cheaper predicate (e.g., fast-path for control.device is Touchscreen) if needed.

private static bool ShouldSkipInitialStateCheck(InputControl control)
{
    // UUM-100125
    // Touch controls intentionally preserve state such as position even when no touch is currently active.
    // During binding re-resolution this can make inactive touches look actuated and cause invalid triggers.
    for (var current = control; current != null; current = current.parent)
    {
        if (current is TouchControl touchControl)
        {
            return !touchControl.isInProgress;
        }
    }

    return false;
}
  • Update review

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr

@u-pr
Copy link
Contributor

u-pr bot commented Mar 3, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Skip check for nested touch children

This only checks control.parent, so bindings to deeper children (e.g.
/primaryTouch/position/x) can still trigger from persisted inactive touch state.
Walk up the full parent chain until a TouchControl is found, and base the skip
decision on that.

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs [1354-1370]

 private static bool ShouldSkipInitialStateCheck(InputControl control)
 {
     // UUM-100125
     // Touch controls intentionally preserve state such as position even when no touch is currently active.
     // During binding re-resolution this can make inactive touches look actuated and cause invalid triggers.
-    if (control is TouchControl touchControl)
+    for (var current = control; current != null; current = current.parent)
     {
-        return !touchControl.isInProgress;
-    }
-
-    if (control.parent is TouchControl parentTouchControl)
-    {
-        return !parentTouchControl.isInProgress;
+        if (current is TouchControl touchControl)
+            return !touchControl.isInProgress;
     }
 
     return false;
 }
Suggestion importance[1-10]: 8

__

Why: The current implementation only checks the control and its direct parent for a TouchControl. For bindings to nested controls, such as <Touchscreen>/primaryTouch/position/x, the check would fail to find the TouchControl and incorrectly skip the suppression logic.

Medium
  • More suggestions

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr

@codecov-github-com
Copy link

codecov-github-com bot commented Mar 3, 2026

Codecov Report

All modified and coverable lines are covered by tests ✅

@@             Coverage Diff             @@
##           develop    #2349      +/-   ##
===========================================
- Coverage    77.90%   77.90%   -0.01%     
===========================================
  Files          476      476              
  Lines        97613    97676      +63     
===========================================
+ Hits         76048    76093      +45     
- Misses       21565    21583      +18     
Flag Coverage Δ
inputsystem_MacOS_2022.3 5.52% <0.00%> (-0.01%) ⬇️
inputsystem_MacOS_2022.3_project 75.39% <100.00%> (-0.01%) ⬇️
inputsystem_MacOS_6000.0 5.30% <0.00%> (-0.01%) ⬇️
inputsystem_MacOS_6000.0_project 77.29% <100.00%> (+<0.01%) ⬆️
inputsystem_MacOS_6000.3 5.30% <0.00%> (-0.01%) ⬇️
inputsystem_MacOS_6000.3_project 77.29% <100.00%> (+<0.01%) ⬆️
inputsystem_MacOS_6000.4 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_MacOS_6000.4_project 77.30% <100.00%> (+<0.01%) ⬆️
inputsystem_MacOS_6000.5 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_MacOS_6000.5_project 77.30% <100.00%> (-0.01%) ⬇️
inputsystem_MacOS_6000.6 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_MacOS_6000.6_project 77.30% <100.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_2022.3_project 75.19% <100.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.0 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.0_project 77.10% <100.00%> (+<0.01%) ⬆️
inputsystem_Ubuntu_6000.3 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.3_project 77.09% <100.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.4 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.4_project 77.11% <100.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.5 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.5_project 77.10% <100.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.6 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.6_project 77.10% <100.00%> (-0.01%) ⬇️
inputsystem_Windows_2022.3 5.52% <0.00%> (-0.01%) ⬇️
inputsystem_Windows_2022.3_project 75.52% <100.00%> (-0.01%) ⬇️
inputsystem_Windows_6000.0 5.30% <0.00%> (-0.01%) ⬇️
inputsystem_Windows_6000.0_project 77.42% <100.00%> (+<0.01%) ⬆️
inputsystem_Windows_6000.3 5.30% <0.00%> (-0.01%) ⬇️
inputsystem_Windows_6000.3_project 77.42% <100.00%> (+<0.01%) ⬆️
inputsystem_Windows_6000.4 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_Windows_6000.4_project 77.42% <100.00%> (-0.01%) ⬇️
inputsystem_Windows_6000.5 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_Windows_6000.5_project 77.42% <100.00%> (-0.01%) ⬇️
inputsystem_Windows_6000.6 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_Windows_6000.6_project 77.42% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
Assets/Tests/InputSystem/CoreTests_Actions.cs 98.15% <100.00%> (+<0.01%) ⬆️
...nputsystem/InputSystem/Actions/InputActionState.cs 92.77% <100.00%> (-0.23%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Addresses U-PR feedback
Previously the code only checked the control and its immediate parent for a TouchControl, which could miss deeper nested touch controls and cause inactive touches to appear actuated during binding re-resolution. Replace the checks with a loop that walks up the control hierarchy and returns based on the first ancestor TouchControl's isInProgress value, preventing invalid triggers from preserved touch state.
…ate-reset' of https://github.com/Unity-Technologies/InputSystem into fix/uum-100125/inactive-touch-drive-phantom-click-on-state-reset
@u-pr
Copy link
Contributor

u-pr bot commented Mar 5, 2026

Persistent review updated to latest commit ea8b067

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant